Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add std.valgrind module #1863

Merged
merged 3 commits into from Mar 11, 2019
Merged

Add std.valgrind module #1863

merged 3 commits into from Mar 11, 2019

Conversation

daurnimator
Copy link
Collaborator

For #1837

@daurnimator daurnimator force-pushed the valgrind branch 3 times, most recently from 0f2e3f4 to 8ba418e Compare December 30, 2018 17:54
//pub fn printf(format: [*]const u8, args: ...) usize {
// return doClientRequestExpr(0,
// ClientRequest.PrintfValistByRef,
// @ptrToInt(format), @ptrToInt(args),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocked on #515

//}


pub fn nonSIMDCall0(func: fn(usize) usize) usize {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if I should make this into a vararg function

std/valgrind/index.zig Outdated Show resolved Hide resolved


// Load PDB debug info for Wine PE image_map.
// pub fn loadPdbDebuginfo(fd, ptr, total_size, delta) void {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea how this function is meant to work or what the args are; it is underdocumented.

std/valgrind/index.zig Outdated Show resolved Hide resolved
std/valgrind/index.zig Outdated Show resolved Hide resolved
@andrewrk
Copy link
Member

andrewrk commented Feb 8, 2019

What kind of testing have you done with this so far?

Here's what I see as the path forward on this, in roughly this order:

  1. debug symbols missing when running with valgrind #896 - send patch to valgrind team fixing the bug so that we don't need the --no-rosegment workaround.
  2. Add language support for valgrind. Auto-enable it for some targets in debug/release-safe modes. It would automatically do the marking memory as undefined, when you assign undefined to it. std.mem.Allocator would then get this for free, without introducing one more barrier to comptime allocators.
  3. The std.valgrind module is generally useful and would be a good candidate to live in the zig standard library. However language support for valgrind will probably emit inline assembly directly, without going through the trouble of a C ABI or parsing .zig code.
  4. More experimentation/research can be done into more integrations with valgrind, potentially even as part of the zig language. (In the same way that it is useful to both zig and valgrind to know that you are setting something to undefined, it could be useful to both zig and valgrind to know that this memory has just been "allocated" or "freed", for example - see the noalias LLVM attribute on return values)

@andrewrk andrewrk added the work in progress This pull request is not ready for review yet. label Feb 8, 2019
@andrewrk andrewrk removed the work in progress This pull request is not ready for review yet. label Feb 18, 2019
\\ xchgl %%ebx,%%ebx
: [_] "={edx}" (-> usize)
: [_] "{eax}" ([]usize{request, a1, a2, a3, a4, a5}),
[_] "0" (default)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constraint code appears to be undocumented. What are you expecting it to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a description on https://gcc.gnu.org/onlinedocs/gcc/Simple-Constraints.html#Simple-Constraints

An operand that matches the specified operand number is allowed. If a digit is used together with letters within the same alternative, the digit should come last.

This number is allowed to be more than a single digit. If multiple digits are encountered consecutively, they are interpreted as a single decimal integer. There is scant chance for ambiguity, since to-date it has never been desirable that ‘10’ be interpreted as matching either operand 1 or operand 0. Should this be desired, one can use multiple alternatives instead.

This is called a matching constraint and what it really means is that the assembler has only a single operand that fills two roles which asm distinguishes. For example, an add instruction uses two input operands and an output operand, but on most CISC machines an add instruction really has only two operands, one of them an input-output operand:

addl #35,r12

Matching constraints are used in these circumstances. More precisely, the two operands that match must include one input-only operand and one output-only operand. Moreover, the digit must be a smaller number than the number of the operand that uses it in the constraint.

I want to double check with LLVM that this is officially supported before depending on it. If not Zig should emit a compile error.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is ready to merge yet. Aside from not passing CI tests, I double checked valgrind.h and the assembly you copied here is marked as being for

  • amd64-{linux,darwin,solaris}
  • x86-{linux,darwin,solaris}

Windows has different inline assembly.

I tried the code and it gave an invalid LLVM module error:

Call parameter type does not match function signature!
  %7 = alloca [6 x i64], align 8
 [6 x i64]  %21 = call i64 asm sideeffect " rolq $$3,  %rdi ; rolq $$13, %rdi\0A rolq $$61, %rdi ; rolq $$51, %rdi\0A xchgq %rbx,%rbx", "={rdx},{rax},0,~{cc},~{memory}"([6 x i64]* %7, i64 %20), !dbg !191
LLVM ERROR: Broken module found, compilation aborted!

As @Sahnvour notes, you can't pass an array directly to inline assembly. It's a bug in zig that it isn't a compile error.

@andrewrk andrewrk added the work in progress This pull request is not ready for review yet. label Feb 19, 2019
std/mem.zig Outdated Show resolved Hide resolved
std/mem.zig Outdated Show resolved Hide resolved
std/mem.zig Outdated Show resolved Hide resolved
std/valgrind/index.zig Outdated Show resolved Hide resolved
@daurnimator
Copy link
Collaborator Author

Windows has different inline assembly.

It didn't seem to when I looked, it just doesn't take that code path in msvc due to using GCC inline assembly?

I tried the code and it gave an invalid LLVM module error

That must be new, I ran+tested the code in this PR.

As @Sahnvour notes, you can't pass an array directly to inline assembly.

Will fix.

It's a bug in zig that it isn't a compile error.

Is there an issue open for that?

@Sahnvour
Copy link
Contributor

Is there an issue open for that?

There's #215 which is broader in scope. Actually from my experience and what @andrewrk told me about it, inline asm has enough support for basic stuff (namely doing syscalls) but that's about it. When you deviate a bit from simple uses, be sure to check with a debug build of Zig to trigger LLVM asserts.

@andrewrk andrewrk removed the work in progress This pull request is not ready for review yet. label Feb 20, 2019
@andrewrk andrewrk dismissed their stale review February 20, 2019 02:45

I'll do another review

@daurnimator
Copy link
Collaborator Author

daurnimator commented Feb 20, 2019

One quick test you can do to show this is working is:

valgrindtest.zig:

const std = @import("std");
const valgrind = std.valgrind; // @import("./std/valgrind/index.zig");

pub fn main() void {
    std.debug.warn("{}\n", valgrind.runningOnValgrind());
}
$ zig build-exe valgrindtest.zig
$ ./valgrindtest 
0
$ valgrind ./valgrindtest 
==3338== Memcheck, a memory error detector
==3338== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3338== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==3338== Command: ./valgrindtest
==3338== 
1
==3338== 
==3338== HEAP SUMMARY:
==3338==     in use at exit: 0 bytes in 0 blocks
==3338==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==3338== 
==3338== All heap blocks were freed -- no leaks are possible
==3338== 
==3338== For counts of detected and suppressed errors, rerun with: -v
==3338== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@andrewrk andrewrk merged commit 7bbccfc into ziglang:master Mar 11, 2019
@daurnimator daurnimator deleted the valgrind branch March 12, 2019 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants